Skip to content

Conversation

DominicGBauer
Copy link
Contributor

@DominicGBauer DominicGBauer commented Oct 15, 2024

Description

Currently, apps do not work as expected when users use an insecure context (e.g. http://127.0.0.2) which results in confusing behaviour for users (e.g. https://discord.com/channels/1138230179878154300/1291056749197131796).

This PR adds a check to confirm that navigator locks are present and throws an error otherwise. This should avoid users experiencing the above mentioned issue.

This also includes vitest updates as they are required to avoid false positives see here (vitest-dev/vitest#6187)

Testing

I've added unit tests to confirm the behaviour

Copy link

changeset-bot bot commented Oct 15, 2024

⚠️ No Changeset found

Latest commit: de55193

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

@rkistner rkistner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the typical cases where navigator.locks is not available?
Could this cause other issues, such as multiple tabs syncing concurrently?

@DominicGBauer
Copy link
Contributor Author

DominicGBauer commented Oct 16, 2024

@rkistner based on this https://discord.com/channels/1138230179878154300/1291056749197131796 the general issue is an unsecure context being used (e.g. http://192.168...) which results in a hard to diagnose issue for users. The idea with this is to warn users when navigator locks are not available and to then have a fallback so they can still continue albeit with more limited functionality.

@rkistner
Copy link
Contributor

Ok, I wasn't aware of the secure context limitation.

Is it feasible to limit this to only "safe" setups, such as when using a SharedWorker? I'm concerned that silent data corruption because of a lack of cross-tab locks could be even more difficult to debug.

@stevensJourney
Copy link
Collaborator

I did some digging and found that WA-SQLite also uses the navigator.locks API internally. From what I could see Navigator locks are currently unavoidable if we use the IDBBatchAtomicVFS VFS which uses WebLocks under the hood.

@DominicGBauer DominicGBauer changed the title WIP chore: add check for navigator locks chore: add check for navigator locks Nov 11, 2024
@DominicGBauer DominicGBauer marked this pull request as ready for review November 11, 2024 08:22
@DominicGBauer DominicGBauer merged commit 6e58b32 into main Nov 11, 2024
6 checks passed
@DominicGBauer DominicGBauer deleted the chore/add-check-for-locks branch November 11, 2024 08:22
@gavin-ob
Copy link

HI there, is this work complete? And if so do you have any idea when it will be released? I think I am in a situation where this would benefit me, I am building a nextjs application and then converting it to ios and android app using capacitor. Powersync seems to work well when the apps are bundled fully, but during the development process I am pointing to local host and using the nextjs instance that is running there, which enables hot reloading of my changes in xcode and android studio, this method doesn't work with powersync, I get an error that mentions navigator locks.

Apologies if this is unrelated, but would be a massive help if your changes solve it! happy to test them on this use case if you can point me in the direction of a package with the changes implemented.

@rkistner
Copy link
Contributor

This is released already, but it only checks that navigator.locks are present, giving a better error message when it isn't.

Even if we removed usage of the locks here, wa-sqlite still uses the same navigator.locks internally, so it is something that is required.

You say you're pointing to local host in development - is that http://localhost:<port>, or for example using your WiFi IP? localhost should work as far as I know, but the WiFi IP won't. At least for Android you can use adb reverse to expose your development server as an actual http://localhost URL on the device. Not sure if there is an equivalent for iOS.

@gavin-ob
Copy link

@rkistner Thanks very much for the response, I will give that a try

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants